fix(core): resolve 16 audit findings — race conditions, design flaws, glue code, and performance issues#529
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses the Core audit findings (#528) by hardening update execution against race conditions, removing duplicated “glue code”, aligning cross-component behaviors (hooks, SSL policy), and improving performance hot spots.
Changes:
- Fixes several race/concurrency issues (process start TOCTOU, re-entrant execution guard, safer directory deletion) and corrects rollback semantics to avoid undoing already-applied versions.
- Removes duplicated logic by extracting shared helpers (OS strategy resolution, blacklist defaults) and unifying SSL policy management through
HttpClientProvider. - Improves performance via cached version parsing, reusable hash helper instance, and temp directory cleanup at startup.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/c#/GeneralUpdate.Core/Strategy/WindowsStrategy.cs | Removes HasExited TOCTOU check after Process.Start. |
| src/c#/GeneralUpdate.Core/Strategy/MacStrategy.cs | Mirrors Windows process-start fix on macOS. |
| src/c#/GeneralUpdate.Core/Strategy/UpdateStrategy.cs | Uses shared OS resolver; aligns hook exception semantics (abort on exception). |
| src/c#/GeneralUpdate.Core/Strategy/OsStrategyResolver.cs | New shared OS strategy/platform resolver to remove duplication. |
| src/c#/GeneralUpdate.Core/Strategy/ClientStrategy.cs | Uses shared OS resolver and shared blacklist defaults helper. |
| src/c#/GeneralUpdate.Core/Strategy/AbstractStrategy.cs | Adds re-entry guard and rollback guard; null-safety for zip deletion. |
| src/c#/GeneralUpdate.Core/Pipeline/HashMiddleware.cs | Reuses a static Sha256HashAlgorithm instance. |
| src/c#/GeneralUpdate.Core/Network/VersionService.cs | Delegates SSL policy and shared client usage to HttpClientProvider. |
| src/c#/GeneralUpdate.Core/GracefulExit.cs | Avoids self-Kill; attempts a gentler self-close path. |
| src/c#/GeneralUpdate.Core/FileSystem/StorageManager.cs | Adds temp cleanup and hardens recursive deletion against concurrent FS changes. |
| src/c#/GeneralUpdate.Core/FileSystem/BlackMatcher.cs | Tightens directory skip matching (prefix-based). |
| src/c#/GeneralUpdate.Core/FileSystem/BlackDefaults.cs | Adds CreatePolicyWithDefaults to centralize blacklist defaulting. |
| src/c#/GeneralUpdate.Core/Download/Orchestrators/DefaultDownloadOrchestrator.cs | Improves semaphore-timeout diagnostics and reduces timeout. |
| src/c#/GeneralUpdate.Core/Download/DownloadPlanBuilder.cs | Caches parsed versions to reduce repeated parsing. |
| src/c#/GeneralUpdate.Core/Configuration/UpdateRequest.cs | Updates XML docs to match actual validation behavior. |
| src/c#/GeneralUpdate.Core/Configuration/Option.cs | Replaces global lock with ConcurrentDictionary.GetOrAdd. |
| src/c#/GeneralUpdate.Core/Bootstrap/GeneralUpdateBootstrap.cs | Removes duplicated bowl shutdown + blacklist init; adds temp cleanup at startup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+250
to
+254
| // Parse timestamp from "generalupdate_yyyy-MM-dd-HHmmss-fff_PID_name" | ||
| // If the PID segment matches the current process, skip it. | ||
| var parts = dirName.Split('_'); | ||
| if (parts.Length >= 4 && int.TryParse(parts[3], out var pid) && pid == currentPid) | ||
| continue; |
Comment on lines
344
to
349
| public static void DeleteDirectory(string targetDir) | ||
| { | ||
| // Enumerate then delete with per-item exception handling. | ||
| // Between enumeration and deletion, concurrent processes may add/remove | ||
| // files — handle these races gracefully instead of crashing. | ||
| foreach (var file in Directory.GetFiles(targetDir)) |
Comment on lines
+96
to
+100
| @@ -97,7 +97,7 @@ public bool IsBlacklistedFormat(string extension) | |||
| /// </remarks> | |||
| public bool ShouldSkipDirectory(string directoryName) | |||
| => _config.Directories?.Any(d => | |||
| directoryName.IndexOf(d, StringComparison.OrdinalIgnoreCase) >= 0) == true; | |||
| directoryName.StartsWith(d, StringComparison.OrdinalIgnoreCase)) == true; | |||
Comment on lines
+72
to
+75
| // GetOrAdd is lock-free on ConcurrentDictionary; the factory runs at most once per key. | ||
| // If the name was already registered with a different type T, the cast below throws, | ||
| // which is far more likely to be a programming error than a runtime concern. | ||
| var raw = _registry.GetOrAdd(name, _ => new Option<T>(name, defaultValue)); |
Comment on lines
+86
to
+92
| /// <summary>Pre-parses versions for a list of assets to avoid repeated Semver.TryParse calls.</summary> | ||
| private static Dictionary<DownloadAsset, SemVersion?> PreParseVersions(IEnumerable<DownloadAsset> assets) | ||
| { | ||
| return assets.ToDictionary( | ||
| a => a, | ||
| a => ParseVersion(a.Version)); | ||
| } |
JusterZhu
force-pushed
the
fix/core-audit-bugs-and-cleanup
branch
from
dd80200 to
31fa0d4
Compare
… glue code, and performance issues This commit addresses the full audit review of GeneralUpdate.Core: **Bug fixes:** - Remove HasExited race condition in WindowsStrategy & MacStrategy (#3,#4) - Only rollback when no version succeeded yet in AbstractStrategy (#5) - Guard against null version.Name in DeleteVersionZip (#6) - Better semaphore timeout logging in DefaultDownloadOrchestrator (#8) - Robust concurrent-safe DeleteDirectory in StorageManager (#10) - Add re-entry guard in AbstractStrategy.ExecuteAsync (#11) **Design improvements:** - Fix XML doc to match actual validation in UpdateRequest.Validate() (#12) - Unify SSL policy: VersionService delegates to HttpClientProvider (#16,#17) - GracefulExit self-shutdown no longer calls Kill() on the current process (#18) - Use StartsWith instead of IndexOf in BlackMatcher.ShouldSkipDirectory (#19) - Replace lock with ConcurrentDictionary.GetOrAdd in Option.ValueOf (#20) **Glue code removal:** - Extract shared BlackDefaults.CreatePolicyWithDefaults() (#21) - Remove duplicate CallSmallBowlHomeAsync from Bootstrap (#22) - Extract shared OsStrategyResolver class (#23) - Make SafeOnBeforeUpdateAsync semantics consistent: exception = abort (#24) **Performance:** - Cache parsed SemVers in DownloadPlanBuilder to avoid repeated parsing (#26) - Reuse Sha256HashAlgorithm as static field in HashMiddleware (#30) - Add CleanupOldTempDirectories() to prevent temp directory accumulation (#31) Co-authored-by: Claude <noreply@anthropic.com>
JusterZhu
force-pushed
the
fix/core-audit-bugs-and-cleanup
branch
from
31fa0d4 to
ee30c85
Compare
- Fix PID parse index in CleanupOldTempDirectories (index 2, not 3) - Wrap all DeleteDirectory enumeration in try/catch for concurrent-safety - Extract directory name in ShouldSkipDirectory to handle both bare names and full paths - Update Option.cs GetOrAdd comment to note factory may run multiple times under contention - Dispose Process handle with 'using' in GracefulExit.CurrentProcessAsync - Guard PreParseVersions against duplicate keys from custom IDownloadSource implementations Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 16 findings from the Core audit (issue #528). Covers race conditions, rollback logic, null-safety, design inconsistencies, repeated/duplicated code, and performance hot-spots.
Changes
Bug Fixes
HasExitedcheck afterProcess.Start()— a fast-starting app can exit before we inspect it, causing a false failure.TryRollback()is now guarded by_appliedAnyVersion— only rollback when no version has been applied yet in this batch.Interlocked.Exchangeguard to prevent corrupt state from concurrent calls.Design Improvements
_sharedClientand_globalSslPolicy; delegates entirely toHttpClientProvider.CurrentProcessAsync()no longer callsKill()on the current process — lets the async stack unwind naturally.IndexOftoStartsWithprefix matching to avoid false-positive directory skips.lock+TryGetValuewithConcurrentDictionary.GetOrAdd.Glue Code Removal
BlackDefaults.CreatePolicyWithDefaults()shared helper.CallSmallBowlHomeAsyncfrom Bootstrap; ClientStrategy already handles it.OsStrategyResolverclass.SafeOnBeforeUpdateAsyncnow returnsfalseon exception (abort), matching ClientStrategy behavior.Performance
Lookup()helper.Sha256HashAlgorithmastatic readonlyfield.CleanupOldTempDirectories()called at bootstrap startup, cleaning dirs older than 24h.Testing
netstandard2.0,net8.0, andnet10.0with 0 errors, 0 warnings.Closes #528